Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: shebangs #3922

Closed
wants to merge 3 commits into from
Closed

fix: shebangs #3922

wants to merge 3 commits into from

Conversation

brandonkal
Copy link
Contributor

The existing shebangs were not portable to Linux (which has no -S option).
This fix enables the shebangs to function in a more portable way.

@brandonkal
Copy link
Contributor Author

Build failure is unrelated (Python format). Requesting a maintainer to trigger it again.

@piscisaureus
Copy link
Member

@brandonkal I restarted CI but it failed at the same point, so I don't think it's unrelated. Can you run format.py locally?

@brandonkal
Copy link
Contributor Author

My bad. The fact python was calling Prettier threw me off. It should pass now.

@brandonkal
Copy link
Contributor Author

@piscisaureus Note that it is still preferrable to not have a semicolon there.
But at least this works in more places than before.

Some shells will print "./catj.ts: 2: ./catj.ts: //: Permission denied" before executing the script.
There is no good way to add a prettier-ignore there either because not all shells accept "//" comments.

@bartlomieju
Copy link
Member

CC @nayeemrmn I remember you were updating shebangs some time ago. Can you take a look?

@nayeemrmn
Copy link
Collaborator

Yeah, the env -S solution is for GNU coreutils 8.30+ or something. But only the relatively newer versions of distros have this. The change here I believe is StackOverflow's solution for full compatibility. I probably should have done this in the first place.

@bartlomieju bartlomieju requested a review from ry February 9, 2020 11:06
#!/usr/bin/env -S deno run --reload --allow-run
#!/bin/sh
":" //; exec /usr/bin/env deno run --reload --allow-run "$0" "$@"
.charAt(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't parse this.. what is happening here?

Copy link
Contributor Author

@brandonkal brandonkal Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The first line tells unix to parse as a shell script line by line.
  2. The : command which is quoted to be valid JavaScript is the "Do nothing beyond expanding arguments and performing redirections." Its only argument is // which is a valid path.
  3. Then we start a new shell statement after the ; inside the JavaScript comment.
  4. exec replaces the shell process with deno passing the desired deno arguments, the script file "$0" and any arguments the user provides "$@"
  5. The .charAt(0); line is required so that Prettier doesn't insert a semicolon after the ":" JavaScript statement. So instead JavaScript executes ":".charAt(0); which is basically a no-op.

@ry
Copy link
Member

ry commented Feb 13, 2020

Thanks for the patch but sorry this is too weird. We need another solution.

@ry ry closed this Feb 13, 2020
@brandonkal
Copy link
Contributor Author

@ry It is weird but it is the only current solution that is portable especially as -S isn't universally available for the env command.

In most environments only one argument can be passed to a program by env.
Another solution is to introduce another deno flag -x https://perldoc.perl.org/perlrun.html#*-x*.

Then the shebang could look like this:

#!/usr/bin/env deno -x
// deno run --reload --allow-run "$0" "$@"

@ry
Copy link
Member

ry commented Feb 14, 2020

I'd rather change how we parse args so we can support all types of env

@brandonkal
Copy link
Contributor Author

@ry So you would say the "-x" solution is good?

@ry
Copy link
Member

ry commented Feb 14, 2020

@brandonkal anything is better than exposing the hack in this patch : )

@brandonkal
Copy link
Contributor Author

Okay. I'll look to add -x if I find the spare cycles.

@brandonkal brandonkal deleted the fix-shebangs branch February 14, 2020 01:00
@nayeemrmn
Copy link
Collaborator

#!/usr/bin/env deno -x

Which platforms does this work on? The Linux limitation is one interpreter /usr/bin/env one arg to that interpreter "deno -x" and then the script name is the next arg.

I tried this and it calls /usr/bin/env "deno -x" <script>, which is wrong.

@brandonkal
Copy link
Contributor Author

Oh yes of course. So it looks like this will remain unsolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants